Fix make_simplified_union interaction with Any #2197

ddfisher · 2016-09-30T00:07:26Z

make_simplified_union had two incorrect interactions with Any that this
fixes:

Any unions containing Any were simplified to Any, which is incorrect.
Any classes that inherited from Any would be removed from the union
by the subclass simplification (because subclasses of Any are
considered subclasses of all other classes due to subtype/compatible
with confusion).

Note that Unions call make_union and not make_simplified_union, so
this doesn't change their behavior directly (unless they interact with
something else which causes them to be simplified, like being part of
an Optional).

make_simplified_union had two incorrect interactions with Any that this fixes: 1) Any unions containing Any were simplified to Any, which is incorrect. 2) Any classes that inherited from Any would be removed from the union by the subclass simplification (because subclasses of Any are considered subclasses of all other classes due to subtype/compatible with confusion). Note that `Union`s call make_union and not make_simplified_union, so this doesn't change their behavior directly (unless they interact with something else which causes them to be simplified, like being part of an `Optional`).

gvanrossum · 2016-09-30T00:43:58Z

mypy/types.py

-                if i != j and is_subtype(tj, ti):
+                tj_is_anylike = (isinstance(tj, AnyType) or
+                                 (isinstance(tj, Instance) and tj.type.fallback_to_any))
+                if i != j and is_subtype(tj, ti) and not tj_is_anylike:


Can you check tj_is_anylike before is_subtype()? It seems perverse to call the latter when you know you're going to reject the result.

See also my comment #2031 (comment) for another potential improvement (though it wouldn't be consistent with preserving Any types in unions, but it would hopefully fix order-dependence).

Guido: I agree -- will swap the ordering.

Jukka: I think it'd be better to fix is_subtype instead.

Actually, I've changed my mind. I don't feel strongly about this, but I think the conditional is moderately more readable if tj_is_anylike comes after the subtype check. tj_is_anylike isn't true often enough for the perf difference to matter.

If you still prefer it otherwise, I'll switch it.

I don't feel strongly either. I don't have any other objections, but I'll wait with merging this until it's clear what Jukka wanted. Or Jukka can merge.

JukkaL · 2016-09-30T09:27:15Z

test-data/unit/check-optional.test

+class C(Any):
+    pass
+x = None  # type: Optional[C]
+x.foo()  # E: Some element of union has no attribute "foo"


Ideas for additional test cases:

Test case for using isinstance with something like Optional[C] (C has an Any base class), and test how the conditional type binder works in that case.

Similar to above (isinstance), but with Union[Any, int] where there is no Any base class.

Test accessing an attribute of Union[Any, int] where there is no Any base class.

Maybe I'm missing something, but those test cases seem completely unrelated to this issue?

This introduces a new sort of union type (one with both Any and non-Any types), and some other code might make assumptions about this and not work correctly with them. I haven't looked at the relevant code, but it's probably as easy to write the tests than to manually validate that everything is okay. In particular, I remember than union-related operations were implemented in a slightly sketchy fashion and thus might have problems (though they are probably fine)... Alternatively, it would be equally fine to have unit tests that just verify that operations on unions still do the right thing -- restrict_subtype_away comes to mind, in particular.

This actually doesn't introduce a new Union type. Explicitly writing Union[A, B] called make_union, not make_simplified_union, so these types already exist.

Fair enough, I remember seeing that at some point. Thus if things are broken, they were so already. I'll probably file a new issue -- some union-related code I browsed looked pretty suspicious.

There was already an issue open for one of the bugs I was worried about: #1720

ddfisher · 2016-10-03T18:14:52Z

@JukkaL is there anything more you'd like to see here before merging?

JukkaL · 2016-10-03T19:17:18Z

About my comment:

Using a real subtype check would certainly be a better fix than what I suggested. However, the current PR seems incomplete in that it special cases Any but doesn't address concerns with other types such as List[Any] vs. List[int]. My suggestion is to modify make_simplified_union to also handle the latter case so that it always returns List[Any] instead of arbitrarily returning List[Any] or List[int], via taking a join of the union items that are compatible with each other. If you are going to implement the actual subtype check in a follow-up PR soon, this is not important, and the suggestion is optional in any case. If you don't plan to implement the real subtype check soon, I'd suggest adding a comment about why the current fix is still incomplete.

The real subtype check might be somewhat non-trivial to implement in a way that handles all the corner cases correctly, so I think that it would be reasonable to make a temporary fix that is still not quite correct but less incorrect than what we currently have, as the required incremental change would be small :-)

About subtypes vs compatibility:

I don't see how is_subtype is broken, other than being wrongly named, based on the current terminology. If we rename it to is_compatible, is it still broken? There currently is no implementation of actual subtype checking (other than is_proper_subtype, which isn't complete). Modifying is_subtype to perform a subtype check is not the right thing to do -- we need a new, separate operation that will again be called is_subtype (perhaps it can share some code with is_compatible). Almost all current uses of is_subtype are correct, as far as I know, and the calls would just need to be updated to call is_compatible instead (or whatever it will be called).

Historically, I used 'is subtype of' to mean what is currently understood as 'is compatible with', and 'is proper subtype of' for what is currently understood as 'is subtype of'. In retrospect that was a bad decision, and I can well see how it's confusing to use 'subtype' to mean something that is quite different from the textbook definition.

JukkaL · 2016-10-04T13:15:24Z

mypy/types.py

-                if i != j and is_subtype(tj, ti):
+                # Attempt to only combine true subtypes by avoiding types containing Any.
+                # TODO: Properly exclude generics and functions.
+                tj_is_anylike = (isinstance(tj, AnyType) or


What if there are multiple Any types in the union to simplify? I think that we should remove duplicates away at least, perhaps by using is_same_type. So Union[Any, Any, int] would be simplified into Union[Any, int].

Edited

gvanrossum · 2016-10-04T17:49:15Z

@JukkaL @ddfisher are you shooting to get this into the 0.4.5 release?

JukkaL · 2016-10-04T18:14:31Z

As we are pretty close to release, I'm fine with postponing this until the next release. Then we could perhaps release a larger set of fixes to unions. But if @ddfisher wants to get this in it doesn't look very risky once the duplicate Any issue is resolved. However, I'd like to have this tested on real-world code, as the union type implementation has a bunch of issues (unrelated to this PR) and this might cause some of them to become more visible.

gvanrossum · 2016-10-04T18:53:28Z

OK let's postpone it. (David agreed off-line.)

JukkaL · 2016-10-11T12:15:02Z

@ddfisher Can you look at simplifying away duplicate Any values in unions? It's worth adding a test case for this.

gvanrossum · 2016-10-28T19:56:21Z

Ping

ddfisher · 2016-11-02T21:48:16Z

To do this right, I'm going to fix up is_proper_subtype.

rwbarton · 2016-11-02T22:01:04Z

BTW, can someone explain what "proper subtype" is supposed to mean in the context of mypy? I never understood it.

ddfisher · 2016-11-02T22:07:24Z

Here's my understanding: in mypy, "subtype" means "compatible with" and "proper subtype" means "subtype".

rwbarton · 2016-11-02T22:29:04Z

Okay, that makes sense. I saw the terminology "compatible with" elsewhere in mypy and thought it was a separate, third concept but I guess it isn't.

JukkaL · 2017-01-19T13:14:06Z

I need something similar for another thing I'm working on, so I'm probably going to do another PR that replaces this. I'll keep this open until I have the other PR ready.

(This is an updated version of #2197 by ddfisher.) make_simplified_union had two incorrect interactions with Any that this fixes: 1) Any unions containing Any were simplified to Any, which is incorrect. 2) Any classes that inherited from Any would be removed from the union by the subclass simplification (because subclasses of Any are considered subclasses of all other classes due to subtype/compatible with confusion). Note that `Union`s call make_union and not make_simplified_union, so this doesn't change their behavior directly (unless they interact with something else which causes them to be simplified, like being part of an `Optional`).

JukkaL · 2017-01-19T16:06:02Z

#2714 is an updated version of this PR -- it handles some additional cases and adds tests.

gvanrossum reviewed Sep 30, 2016

View reviewed changes

JukkaL reviewed Sep 30, 2016

View reviewed changes

Add clarifying comment

786877a

JukkaL reviewed Oct 4, 2016

View reviewed changes

gvanrossum changed the title ~~Fix make_simplified_union interaction with Any~~ [waiting for author & conflict] Fix make_simplified_union interaction with Any Oct 13, 2016

gvanrossum mentioned this pull request Nov 2, 2016

Generic type aliases #2378

Merged

gvanrossum changed the title ~~[waiting for author & conflict] Fix make_simplified_union interaction with Any~~ Fix make_simplified_union interaction with Any Nov 9, 2016

ilevkivskyi mentioned this pull request Nov 13, 2016

Keep Any when restricting Union in isinstance #2446

Merged

JukkaL mentioned this pull request Jan 19, 2017

Union of a class with Any base and None is sometimes None #2712

Closed

JukkaL mentioned this pull request Jan 19, 2017

Fix make_simplified_union interaction with Any #2714

Closed

JukkaL closed this Jan 19, 2017

gvanrossum deleted the fix-simplified-union branch January 27, 2017 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix make_simplified_union interaction with Any #2197

Fix make_simplified_union interaction with Any #2197

ddfisher commented Sep 30, 2016

gvanrossum Sep 30, 2016

JukkaL Sep 30, 2016

ddfisher Sep 30, 2016

ddfisher Sep 30, 2016

gvanrossum Sep 30, 2016

JukkaL Sep 30, 2016

ddfisher Sep 30, 2016

JukkaL Oct 3, 2016

ddfisher Oct 3, 2016

JukkaL Oct 4, 2016

JukkaL Oct 4, 2016

ddfisher commented Oct 3, 2016

JukkaL commented Oct 3, 2016

JukkaL Oct 4, 2016 •

edited

Loading

gvanrossum commented Oct 4, 2016

JukkaL commented Oct 4, 2016

gvanrossum commented Oct 4, 2016 via email

JukkaL commented Oct 11, 2016

gvanrossum commented Oct 28, 2016

ddfisher commented Nov 2, 2016

rwbarton commented Nov 2, 2016

ddfisher commented Nov 2, 2016

rwbarton commented Nov 2, 2016

JukkaL commented Jan 19, 2017

JukkaL commented Jan 19, 2017

Fix make_simplified_union interaction with Any #2197

Fix make_simplified_union interaction with Any #2197

Conversation

ddfisher commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddfisher commented Oct 3, 2016

JukkaL commented Oct 3, 2016

JukkaL Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

gvanrossum commented Oct 4, 2016

JukkaL commented Oct 4, 2016

gvanrossum commented Oct 4, 2016 via email

JukkaL commented Oct 11, 2016

gvanrossum commented Oct 28, 2016

ddfisher commented Nov 2, 2016

rwbarton commented Nov 2, 2016

ddfisher commented Nov 2, 2016

rwbarton commented Nov 2, 2016

JukkaL commented Jan 19, 2017

JukkaL commented Jan 19, 2017

JukkaL Oct 4, 2016 •

edited

Loading